Skip to content

Support flattened externally tagged enums with default attributes #2687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support flattened externally tagged enums with default attributes #2687

wants to merge 2 commits into from

Conversation

@andrewtoth
Copy link

@andrewtoth andrewtoth commented Feb 2, 2024 •
edited
Loading

This adds a custom error type for FlatMapDeserializer, which distinguishes the specific no variant of enum {name} found in flattened data error.

This error is then used in deserialize_map in serde_derive to match on the returned error, and if the field or container has a default attribute it uses the expr_is_missing to provide a default value.

Fixes #1626

@andrewtoth andrewtoth marked this pull request as draft February 2, 2024 05:41
@andrewtoth andrewtoth marked this pull request as ready for review February 2, 2024 20:25
@andrewtoth andrewtoth force-pushed the andrew/default-flatten branch 2 times, most recently from 2bad167 to 6acbe14 Compare February 2, 2024 22:43
@andrewtoth
Copy link
Author

The tests fail due to trybuild using an older version of serde or serde_derive. This PR only modifies API in the private mod, and in serde_derive. However, if after this patch both serde and serde_derive are not upgraded in lockstep then this will break at compile time. I'm not completely sure then, if this can be considered a non-breaking change? If all packages using serde and serde_derive in a project upgrade in lockstep then there is no external breaking change. Is it a requirement that serde_derive and serde be backwards compatible with older versions of each other?

Mingun
Mingun reviewed Feb 3, 2024
View reviewed changes
Copy link
Contributor

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach! I think, you need to add tests with cases from mentioned issues or even better generalize it and cover all possible variants where FlatMapDeserializer is used

@andrewtoth andrewtoth force-pushed the andrew/default-flatten branch 2 times, most recently from 5f9746c to 9b7b794 Compare February 3, 2024 19:31
@andrewtoth
Copy link
Author

@Mingun thank you for your review and suggestions! I implemented them both.
As for tests, I will happily write them but first could you confirm that the approach here will be accepted?

@andrewtoth andrewtoth changed the title Support flattened enums with default field attributes Support flattened enums with default attributes Feb 3, 2024
@andrewtoth andrewtoth force-pushed the andrew/default-flatten branch from 9b7b794 to 0c1b047 Compare February 3, 2024 19:47
@Mingun
Copy link
Contributor

Mingun commented Feb 3, 2024

I don't know, I am the same external contributor as you. Judging by the general reaction of maintainers, one should hardly expect any attention in the near future

@andrewtoth andrewtoth changed the title Support flattened enums with default attributes Support flattened externally tagged enums with default attributes Feb 6, 2024
@johnmave126
Copy link

It is interesting that I went on to try to solve another problem revolving flatten in #2751 and we actually ended up on very similar approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Mingun Mingun Mingun left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

#[serde(flatten)] doesn't work with #[serde(default)] for externally tagged enums

3 participants

@andrewtoth @Mingun @johnmave126